Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intel x86 Mac Cpu Fix #1955

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

alexbeattie42
Copy link

Fix for this issue

@google-cla
Copy link

google-cla bot commented May 3, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@@ -102,6 +102,9 @@ def _cpu_string(*, cpu, platform_type, settings = {}):
return "ios_sim_arm64"
return "ios_x86_64"
if platform_type == "macos":
host_cpu = settings["//command_line_option:host_cpu"]
if host_cpu == "darwin":
return "darwin"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be below all the other conditions in this block, since --cpu and --macos_cpus should take precedence over this logic

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried moving it below the cpu and macos_cpu options and both failed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this order mean users couldn't cross compile things then for darwin_arm64?

Copy link
Author

@alexbeattie42 alexbeattie42 May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not able to test darwin_arm64 since I don't have an apple silicone mac, but I think it's host cpu value should report as darwin_arm64 so it would not match darwin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right but if you are on your intel mac, where the host_cpu is darwin, and you pass --cpu=darwin_arm64 to produce an output for an arm64 mac, i think this condition would take precedence

Copy link
Author

@alexbeattie42 alexbeattie42 May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that this change does break apple silicone mac. I'm also facing another problem with building on apple silicone. Is there a better approach that could be used to solve this problem without breaking things for the above conditions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'd probably need to check the times we return darwin_x86_64 and return darwin in the case that the host CPU is just darwin, and not affect the return value otherwise

@alexbeattie42 alexbeattie42 force-pushed the master branch 2 times, most recently from eb8c548 to 2d8d143 Compare May 5, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants